Update OrchestratorWorkflow tool docstring #456
Conversation
WalkthroughUpdated examples/temporal/orchestrator.py to make the run_orchestrator app_ctx parameter optional (default None) and adjusted the docstring accordingly. Runtime behavior remains the same by falling back to app.context when app_ctx is not provided. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Orchestrator as run_orchestrator
participant App as app
participant Ctx as AppContext
Caller->>Orchestrator: run_orchestrator(input, app_ctx=None|Ctx)
alt app_ctx provided
Orchestrator->>Ctx: use provided app_ctx
else app_ctx not provided
Orchestrator->>App: access app.context
App-->>Orchestrator: default context
end
Orchestrator-->>Caller: result (str)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rholinshead
requested review from
alienzach,
jtcorbett,
petersonbill64 and
saqadri
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| Args: | ||
| input: Task description or instruction text. | ||
| app_ctx: Optional application context for the workflow. | ||
There was a problem hiding this comment.
The docstring line describing the app_ctx parameter is being removed, but the parameter still exists in the function signature. This creates inconsistent documentation where the function has an app_ctx parameter but no documentation for it. The docstring should be updated to describe the app_ctx parameter with its new default value behavior, not removed entirely.
| :param app_ctx: Application context (defaults to None) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/temporal/orchestrator.py (2)
35-37: Fix docstring return type to match implementationDocstring says “WorkflowResult” but the function returns a
strfromorchestrator.generate_str(...).Apply:
- Returns: - A WorkflowResult containing the processed data + Returns: + str: The generated orchestration output.
39-41: Guard filesystem args and avoid duplicate cwd entries
- Use explicit
Nonecheck to avoid surprise ifAppContextever defines falsy semantics.- Avoid KeyError if “filesystem” server is absent.
- Prevent duplicate cwd entries on repeated calls.
Apply:
- context = app_ctx or app.context - context.config.mcp.servers["filesystem"].args.extend([os.getcwd()]) + context = app_ctx if app_ctx is not None else app.context + fs = context.config.mcp.servers.get("filesystem") + if fs is not None: + cwd = os.getcwd() + if cwd not in fs.args: + fs.args.append(cwd)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/temporal/orchestrator.py(1 hunks)
🔇 Additional comments (2)
examples/temporal/orchestrator.py (2)
28-28: Makingapp_ctxoptional is fineDefaulting
app_ctxtoNonealigns with the intent to not require callers to pass internal context.
27-28: app_ctx is already filtered from public tool schemas — no action required.Tool-registration explicitly drops/filters an app_ctx parameter when generating function signatures/schemas (see src/mcp_agent/server/app_server.py and src/mcp_agent/server/tool_adapter.py); src/mcp_agent/app.py also detects AppContext parameters.
2 participants
Description
Remove reference to internal app_ctx argument
Summary by CodeRabbit
Improvements
Documentation